-
Notifications
You must be signed in to change notification settings - Fork 4
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
refactor: implementation for consuming internal or external artifactory #442
base: main
Are you sure you want to change the base?
Conversation
@JoseRamonRodriguez this is PR that can be tested for external use case with saf desktop 1.0.rc3 for now. This will be finally needed to merge with #436 where solution template is being updated by @iazehaf for upgrade to saf-desktop 1.0.0 and other upgrades. |
@JoseRamonRodriguez @mayur-lankeshwar |
@iazehaf This is a good illustration of the issue we discussed yesterday for the 'source of truth' for the setup-environment.py script. Looking at the changes proposed here, they differ from the latest available in the I also want to highlight that the copy in common-files appears to have a typo introduced by this PR: https://github.com/ansys-internal/solutions-common-files/pull/2 Note the difference between the env var here - @jacobevansgit can you confirm if propagatebot is configured to monitor the setup-environment script in common-files? At quick glance, it looks like yes: https://github.com/ansys-internal/solutions-common-files/blob/d983a4c067c4f280f77d60c6a9fd728e55ea5e46/.github/workflows/propagate_bot.yml#L52-L59 Context: https://github.com/ansys-internal/solution-applications-maintainers/issues/18 |
…suadhyay/func/consume-external-artifactory
…github.com/ansys/ansys-templates into suadhyay/func/consume-external-artifactory
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
A few minor suggestions/renames for clarity. However, I think my last point about how the TOML file is being updated is important to discuss/resolve.
raise Exception(f"Unknown private source {source['name']} with url {source['url']}.") | ||
token = os.environ.get("PYANSYS_PYPI_PRIVATE_PAT") | ||
elif source["url"] == "https://artifactory.ansys.com/artifactory/api/pypi/saf_pypi/simple/": | ||
token = os.environ.get("ARTIFACTORY_PRIVATE_PYPI_PAT") |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
We should be consistent with the ENV vars - notice the order of the pyansys and solutions vars are PYPI_PRIVATE
and the new one is PRIVATE_PYPI
# Declare credentials for private sources | ||
for source in private_sources: | ||
print(f"Declare credentials for {source['name']}") | ||
USERNAME = "PAT" |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
USERNAME = "PAT" | |
username = "PAT" |
# Declare credentials for private sources | ||
for source in private_sources: | ||
print(f"Declare credentials for {source['name']}") | ||
USERNAME = "PAT" | ||
token = None | ||
if source["name"].lower() == "pypi": | ||
continue | ||
elif source["url"] == "https://pkgs.dev.azure.com/pyansys/_packaging/pyansys/pypi/simple/": | ||
token = os.environ["PYANSYS_PYPI_PRIVATE_PAT"] |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
token = os.environ["PYANSYS_PYPI_PRIVATE_PAT"] | |
token = os.environ.get("PYANSYS_PYPI_PRIVATE_PAT") |
# Create Poetry environment variable | ||
os.environ[f"POETRY_HTTP_BASIC_{source_name}_USERNAME"] = "PAT" | ||
os.environ[f"POETRY_HTTP_BASIC_{source_name}_PASSWORD"] = token | ||
if token and USERNAME: |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This should probably have an else
that displays an error
@@ -932,6 +948,66 @@ def install_dotnet_linux_dependencies(): | |||
shell=True, | |||
) | |||
|
|||
def check_if_user_is_internal_or_external(): | |||
private_pypi_pats = [ | |||
os.environ.get("PYANSYS_PRIVATE_PYPI_PAT"), |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
These values are duplicated a few times in this file, so we should probably pull them out as variables
os.environ.get("PYANSYS_PRIVATE_PYPI_PAT"), | ||
os.environ.get("SOLUTIONS_PRIVATE_PYPI_PAT") | ||
] | ||
is_internal = any(private_pypi_pats) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
is_internal = any(private_pypi_pats) | |
internal_source_configured = any(private_pypi_pats) |
'pyansys-private-pypi': {'url': 'https://pkgs.dev.azure.com/pyansys/_packaging/pyansys/pypi/simple/', 'priority': 'supplemental'}, | ||
'artifactory-private-pypi': {'url': 'https://artifactory.ansys.com/artifactory/api/pypi/saf_pypi/simple/', 'priority': 'supplemental'} | ||
} | ||
private_sources = get_private_sources(DEPENDENCY_MANAGER_PATHS["common"]["configuration_file"]) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
private_sources = get_private_sources(DEPENDENCY_MANAGER_PATHS["common"]["configuration_file"]) | |
toml_private_sources = get_private_sources(DEPENDENCY_MANAGER_PATHS["common"]["configuration_file"]) |
} | ||
private_sources = get_private_sources(DEPENDENCY_MANAGER_PATHS["common"]["configuration_file"]) | ||
private_sources = [source for source in private_sources if source['name'] != 'PyPI'] | ||
internal_sources = {'solutions-private-pypi', 'pyansys-private-pypi'} |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The internal vs. private naming is very confusing to me - shouldn't they all be private?
Also, the external part is equally confusing because you still need to be authenticated. For naming, we should either stick to the source (internal/external) or the access (private/public)
else: | ||
sources_to_add |= external_sources - all_sources | ||
|
||
commands = [ |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Personally, I don't like this at all. This means that the TOML file isn't really deterministic - it depends on whatever ENV vars are set when you run the setup script. I think this will result in a lot of unnecessary confusion as developers commit versions of the TOML file switching back and forth between solutions/pyansys vs. artifactory.
I don't know what the requirements are for the artifactory access, so it is difficult for me to propose an alternative solution. I know portal uses 2 TOML files, which I also don't like.
My only other suggestion is to dynamically switch from solutions/pyansys to artifactory during the build/package/release process in CI. However, this would need to be configurable because there may be situations where we want to build a package for internal consumption that depends on prerelease package versions that aren't available in artifactory.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thinking out loud - another option is to test how poetry handles private repositories that aren't authenticated. If this is handled gracefully, we could simply add all the potential sources as supplemental sources and let poetry figure out which ones are authenticated properly.
@nick-marnik, correct, however sadly it was not run on the most recent PR change to What your link shows, is the repos that will receive PRs when the bot is run. |
I think we discussed this before - but in situations where we want propagate bot to monitor a given file, can we skip the entire checklist/label/merge part and simply automate propagate bot to run when any PR affecting a given file is merged? |
Description
Internal and External user must be able to consume internal or external artifactory respectively for downloading packages.
Related Tickets
QA Instructions, Screenshots, Recordings
Steps for internal users to consume packages:
Steps for external users to consume packages: